-
Notifications
You must be signed in to change notification settings - Fork 605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use filtered informer to watch OIDC service accounts #7527
Conversation
|
Welcome @yijie-04! It looks like this is your first PR to knative/eventing 🎉 |
Hi @yijie-04. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@yijie-04 Replying your DM here so others can also benefit from this! Usually when you complete the EasyCLA authorization, check in the GitHub action should turn green immediately. If that doesn't change, there is probably something wrong. Please make sure the github account you used for this PR and the one you used with CLA are the same. Let me know if that helps! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
Thanks @yijie-04 for the PR! Just left few comments. PTAL whenever you have time!
unfiltered service account informers for OIDC service accounts are currently used in multiple places. Most of the fixed Support auto generation of XYZ identity service account and expose in AuthStatus issues in https://github.com/orgs/knative/projects/66/views/4 will probably use it
As @creydr mentioned, there are multiple places are using the unfiltered service account informers. Could you try to update them to use filtered informers too?
Great work Yijie, and please let me know if you need any help!
@yijie-04 Hello Yijie! Are there anything that blocking you? Any help I can provide you with? |
@Leo6Leo Thank you for checking in! It took me a bit of time to debug the unit-tests and it turned out to be simpler than I thought! I'm looking at the other tests right now and it seems that I'm receiving the |
@yijie-04 Hey Yijie, no problem, that's happened to me a lot too! Replying to your question, you can always click into the And as prow suggested, you will need to rebase the PR as some newly merged changes on the main branch is conflicting with your current changes. See the screenshot on how to find the lmk if you have any other questions! |
@yijie-04 There are still some goimports issue in your PR. To fix the formatting issue, here are some practical commands you should be aware of:
When you are running these commands, you should be careful that we don't want to format the files in the And there is a convenient tool that I have made to combine the gofmt and goimport to a single command. Take a look at the source code to understand how that work and Just make sure run this script every time before you commit! |
/retest-required |
@Cali0707 @Leo6Leo Got it, thank you! I just finished formatting the files and I'll wait to see the test results. I do have some questions as I was formatting the code:
Thanks a lot! |
Hey @yijie-04, Great to hear that you've managed to format the files!! Let's address your questions:
In short, while Hope this clarifies your doubts! Don't hesitate to reach out if you have more questions! |
/retest-required |
@Leo6Leo That makes a lot more sense, thank you so much for the clarification! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yijie-04 just a few nits and then this should be good to merge. Specifically, it looks like in the unit test files you are still having the label values as an empty string instead of "enabled". It doesn't seem to be breaking anything currently, but for the sake of keeping the unit tests close to the actual code I think we should probably fix those before we merge this PR. This may break a couple unit tests which are expecting the label value to be "", but you should be able to just go through those and switch it to "enabled"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thanks @yijie-04 for all of your work on this! 🎉🎉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, yijie-04 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@yijie-04 Congrats on the merging! Thanks for the effort you put into this and hope to see more contributions from you in Knative community! |
* controller.go changed * knative#7320 WIP * WIP: Testing filtered informer (knative#7341) * unit test passed * Revert "Merge remote-tracking branch 'otherfork/main' into main" This reverts commit 94cd51b, reversing changes made to 0bf2982. * Removed comments * Changed to filtered informer for Subscription identity service account * Changed to filtered informer for Sequence service accounts * Changed to filtered informer for Parallel identity service accounts * Changed to filtered informer for APIServerSource identity service account * fixed unit tests * added label selector for mtchannel_broker * added filtered informer for sinkbinding identity service accounts * added OIDC label selector in webhook * added filtered informer for containersource service accounts * added filtered informer for pingsource service accounts * added OIDC label selector in apiserver ctx * added OIDC label selector in broker/filter * added OIDC label selector in broker/ingress * added OIDC label selector in in_memory/channel_dispatcher * added OIDC label selector in mtping * fixed unit test issues for pingsource * fixed unit test for container source * formatted files * updated service account informer in apiserversource * updated service account informers in other places * small typo fix * added actual value for OIDC label * added a valid value for OIDClabelkey * changed references of OIDCLabelKey * fixed import path problem * changed OIDCLabelSelector in all main.go files * changed instances of OIDCLabelSelector in controller and controller test files * deleted OIDC related labels from register.go * fixed formatting issues * Added value for OIDCLabelKey --------- Co-authored-by: Scott <scottprotoss@gmail.com>
…y, if SA references a trigger for correct broker class (#592) * Use filtered informer to watch OIDC service accounts (knative#7527) * controller.go changed * knative#7320 WIP * WIP: Testing filtered informer (knative#7341) * unit test passed * Revert "Merge remote-tracking branch 'otherfork/main' into main" This reverts commit 94cd51b, reversing changes made to 0bf2982. * Removed comments * Changed to filtered informer for Subscription identity service account * Changed to filtered informer for Sequence service accounts * Changed to filtered informer for Parallel identity service accounts * Changed to filtered informer for APIServerSource identity service account * fixed unit tests * added label selector for mtchannel_broker * added filtered informer for sinkbinding identity service accounts * added OIDC label selector in webhook * added filtered informer for containersource service accounts * added filtered informer for pingsource service accounts * added OIDC label selector in apiserver ctx * added OIDC label selector in broker/filter * added OIDC label selector in broker/ingress * added OIDC label selector in in_memory/channel_dispatcher * added OIDC label selector in mtping * fixed unit test issues for pingsource * fixed unit test for container source * formatted files * updated service account informer in apiserversource * updated service account informers in other places * small typo fix * added actual value for OIDC label * added a valid value for OIDClabelkey * changed references of OIDCLabelKey * fixed import path problem * changed OIDCLabelSelector in all main.go files * changed instances of OIDCLabelSelector in controller and controller test files * deleted OIDC related labels from register.go * fixed formatting issues * Added value for OIDCLabelKey --------- Co-authored-by: Scott <scottprotoss@gmail.com> * Reconcile trigger on OIDC service account changes only, if SA references a trigger for correct broker class (knative#7849) * Reconcile trigger on OIDC service account changes only, if SA references a trigger for correct broker class * Run goimports and gofmt * Remove deprecated use of pointer.Bool(v) and switch to prt.Bool(v) --------- Co-authored-by: Yijie Wang <147119743+yijie-04@users.noreply.github.com> Co-authored-by: Scott <scottprotoss@gmail.com>
…native#7527) Signed-off-by: Calum Murray <cmurray@redhat.com>
* Watch only our own OIDC-related secrets (#8070) Filter OIDC secrets Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * ./hack/update-deps.sh Signed-off-by: Calum Murray <cmurray@redhat.com> * fix: serviceaccountInformer -> oidcServiceaccountInformer Signed-off-by: Calum Murray <cmurray@redhat.com> * fix: add oidc label selector to main contexts (partial cherry pick of #7527) Signed-off-by: Calum Murray <cmurray@redhat.com> * fix: don't use filtered sa informer when sa is not labelled Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Signed-off-by: Calum Murray <cmurray@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Fixes #7341
Proposed Changes
Pre-review Checklist
Release Note
Docs